umu-launcher[-unwrapped]: init at 1.1.4 #369259

diniamo · 2024-12-29T23:33:38Z

Things done

Add a 👍 reaction to pull requests you find important.

diniamo · 2024-12-29T23:35:39Z

Oh yeah, the build fails because of #366359, I've been working on this with doCheck = false in hatch's package definition.

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

pkgs/by-name/um/umu-launcher/package.nix

pkgs/by-name/um/umu-launcher-unwrapped/makefile-installer-prefix.patch

pkgs/by-name/um/umu-launcher-unwrapped/no-umu-version-json.patch

beh-10257 · 2025-01-07T19:27:09Z

do not look at it if you care about your eyesight

instead of saying this type of stuff why don't you tell me what are your complaints and actually make an improvement

diniamo · 2025-01-07T19:40:22Z

Sorry. The improvement is this package, since I saw more value in upstreaming a proper package to nixpkgs compared to fixing the problems in the flake.

diniamo · 2025-01-07T19:43:35Z

On another note, @MattSturgeon please mark the threads as resolved if they are.

Also, should I add you and @beh-10257 as maintainers to the package (assuming you're already in the maintainers list)?

beh-10257 · 2025-01-07T19:45:45Z

the flake is always gonna be more up to date so there is value in that
yes add me to the maintainers list
can you list the improvements so I can make the flake better like I'm trying to make things better here so cooporate

MattSturgeon · 2025-01-07T19:50:45Z

On another note, @MattSturgeon please mark the threads as resolved if they are.

I don't have permission to do that. I think as PR author you can? Otherwise someone with write-access must do it.

diniamo · 2025-01-07T19:51:43Z

Huh? You should be able to resolve your own reviews. Well in that case, do you want to add anything else or can I resolve them?

diniamo · 2025-01-07T19:59:29Z

...can you list the improvements so I can make the flake better...

@beh-10257 most of them you can get by replacing the package definitions entirely with the ones from this PR, while adapting them for the flake (make sure to tell me about anything that's missing from the packages in this PR along with a good reasoning as well, so I can add them too; eg. optional dependencies, since I couldn't find anything about those, but I did see some in the Nix package). Once you do that, I'll PR the rest of my suggestions, since that's easier than explaining.

MattSturgeon · 2025-01-07T20:11:20Z

Huh? You should be able to resolve your own reviews.

From Resolving conversations:

You can resolve a conversation in a pull request if you opened the pull request or if you have write access to the repository where the pull request was opened.

Well in that case, do you want to add anything else or can I resolve them?

Most of my threads were more discussion than feedback. If you feel that they are resolved, then feel free to mark as such.

the flake is always gonna be more up to date so there is value in that
can you list the improvements so I can make the flake better like I'm trying to make things better here so cooporate

I agree it'd be nice if the flake and nixpkgs were kept in sync. This will likely need breaking changes though.

One way that could be done would be to have the flake base itself on the nixpkgs impl, simply overriding src to point to the upstream repo. E.g:

# flake outputs
{
  overlays.default = final: prev: {
    umu-launcher-unwrapped = prev.umu-launcher-unwrapped.overrideAttrs (old: {
      # TODO: would be nice to filter out irrelevant dirs,
      # to avoid them invalidating build caches
      # e.g. using filesets
      src = ../../.;

      # TODO: version should also be something sensible
      version = "?";
    });
  };
}

One way to avoid that being an invisibly breaking change, would be to add a new flake.nix to the root of the upstream repo.

By keeping the old flake at its existing location (with a warning), users would have time to migrate to the new flake at the new location.

That way a) end-users don't need ?dir=packaging/nix and b) the old flake can be left as-is for a period of migration.

diniamo · 2025-01-07T20:28:33Z

Not needing ?dir=packaging/nix would be the breaking change, as it's been there previously. Also yeah, an override-based approach would be nice.

LovingMelody · 2025-01-11T17:08:49Z

Please add me to the maintainers list as well (fuzen)

diniamo · 2025-01-11T18:03:24Z

Added the 2 of you, but I couldn't find @beh-10257

LovingMelody · 2025-01-17T01:16:52Z

pkgs/by-name/um/umu-launcher/package.nix

@@ -0,0 +1,15 @@
+{ buildFHSEnv, umu-launcher-unwrapped }:


May be worth copying heroic

nixpkgs/pkgs/games/heroic/fhsenv.nix

Lines 4 to 5 in 235caa7

extraPkgs ? pkgs: [ ],

extraLibraries ? pkgs: [ ],

To make it easier for end-users to add extra packages to the FHS environment by overriding.

This could be done in a follow-up PR if preferred

Yes I what the code does, but in what scenario would that be useful

I assume heroic launcher also supports native games, in which case it would make sense, but as far as I know, umu is for windows games only

Needed for passing graphics and fixing a cursor issue. Graphics is needed for dxvk-nvapi to work

nixpkgs/nixos/modules/programs/steam.nix

Lines 51 to 57 in e24b4c0

extraLibraries = pkgs: let

prevLibs = if prev ? extraLibraries then prev.extraLibraries pkgs else [ ];

additionalLibs = with config.hardware.graphics;

if pkgs.stdenv.hostPlatform.is64bit

then [ package ] ++ extraPackages

else [ package32 ] ++ extraPackages32;

in prevLibs ++ additionalLibs;

I don't know, I feel like these make more sense to be done with overrides.

I’m pretty new to nixpkgs and all but I’m going to second @LovingMelody on this one, I think adding the options to enable easier overrides would be a good thing, particularly since I suspect that will be something done often as shown with steam there. As mentioned though having it be in a separate PR is fine I think.

diniamo · 2025-01-17T05:08:42Z

Fixed formatting

MattSturgeon

Oh yeah, the build fails because of #366359, I've been working on this with doCheck = false in hatch's package definition.

The fix (#371743) is currently in staging-next. Not sure how long until that'll be merged into master.

pkgs/by-name/um/umu-launcher-unwrapped/no-umu-version-json.patch

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

pkgs/by-name/um/umu-launcher/package.nix

MattSturgeon · 2025-01-17T12:09:22Z

Also, should I add you and @beh-10257 as maintainers to the package (assuming you're already in the maintainers list)?

yes add me to the maintainers list

Added the 2 of you, but I couldn't find @beh-10257

It seems they aren't in the list. It'd be great to have them, but they'll have to do that themselves in a follow up PR.

@beh-10257 see the relevant documentation:

How to become a maintainer
comments at the top of maintainer-list.nix
Commit conventions

diniamo · 2025-01-18T16:39:44Z

Sigh... that should be fine for the remaining reviews.

I'm not willing to add the extra* stuff however, unless another another committer wants them, as I really don't think they make sense, when they are only required for workarounds (that can also just as easily be done with overrideAttrs).

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

diniamo · 2025-01-19T05:09:53Z

@MattSturgeon does that look ok?

MattSturgeon · 2025-01-19T05:18:07Z

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

+    # Ideally, a change should be upstreamed, that adds both arguments
+    # if their respective variables are set.


Suggested change

# Ideally, a change should be upstreamed, that adds both arguments

# if their respective variables are set.

# See https://github.com/Open-Wine-Components/umu-launcher/pull/343

I've opened an upstream PR, so let's link to it 🙃

It's merged now, so we can safely use fetchpatch2:

diff --git a/pkgs/by-name/um/umu-launcher-unwrapped/makefile-installer-prefix.patch b/pkgs/by-name/um/umu-launcher-unwrapped/makefile-installer-prefix.patch deleted file mode 100644 index 890d6537e33c..000000000000 --- a/pkgs/by-name/um/umu-launcher-unwrapped/makefile-installer-prefix.patch +++ /dev/null @@ -1,13 +0,0 @@ -diff --git a/Makefile.in b/Makefile.in -index b82053d..3e4379e 100644 ---- a/Makefile.in -+++ b/Makefile.in -@@ -90,7 +90,7 @@ umu-dist: $(OBJDIR)/.build-umu-dist - umu-dist-install: umu-dist - $(info :: Installing umu ) - install -d $(DESTDIR)$(PYTHONDIR)/$(INSTALLDIR) -- $(PYTHON_INTERPRETER) -m installer --destdir=$(DESTDIR) $(OBJDIR)/*.whl -+ $(PYTHON_INTERPRETER) -m installer --prefix=$(PREFIX) $(OBJDIR)/*.whl - - ifeq ($(FLATPAK), xtrue) - umu-install: version-install umu-dist-install diff --git a/pkgs/by-name/um/umu-launcher-unwrapped/package.nix b/pkgs/by-name/um/umu-launcher-unwrapped/package.nix index 357ef7a5d806..1f97d0c31580 100644 --- a/pkgs/by-name/um/umu-launcher-unwrapped/package.nix +++ b/pkgs/by-name/um/umu-launcher-unwrapped/package.nix @@ -1,6 +1,7 @@ { python3Packages, fetchFromGitHub, + fetchpatch2, lib, bash, hatch, @@ -22,10 +23,12 @@ python3Packages.buildPythonPackage rec { # This should fail, and be removed for releases after 1.1.4, # see https://github.com/Open-Wine-Components/umu-launcher/pull/289 (replaceVars ./no-umu-version-json.patch { inherit version; }) - # Patch the python installer call to use --prefix instead of --destdir - # Ideally, a change should be upstreamed, that adds both arguments - # if their respective variables are set. - ./makefile-installer-prefix.patch + # Patch the python installer call to support PREFIX + # see https://github.com/Open-Wine-Components/umu-launcher/pull/343 + (fetchpatch2 { + url = "https://github.com/Open-Wine-Components/umu-launcher/pull/343.diff"; + hash = "sha256-+h0ao2WUVUUR/AHEtGj5IAIUVivfAc+asnr0632hN74="; + }) ]; nativeBuildInputs = [

MattSturgeon · 2025-01-19T09:04:48Z

Oh yeah, the build fails because of #366359, I've been working on this with doCheck = false in hatch's package definition.

The fix (#371743) is currently in staging-next. Not sure how long until that'll be merged into master.

#371743 has finally made it to master 🎉

MattSturgeon

Installed & tested locally, seems to be working.

Thanks for your efforts on this!

Test out the nixpkgs PR instead of using the upstream flake NixOS/nixpkgs#369259

khaneliman · 2025-01-19T15:56:22Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369259

`x86_64-linux`

✅ 2 packages built:

umu-launcher
umu-launcher-unwrapped

`aarch64-linux`

✅ 2 packages built:

umu-launcher
umu-launcher-unwrapped

diniamo · 2025-01-19T16:20:34Z

Thanks

diniamo force-pushed the umu-launcher branch from ccb8609 to 6972537 Compare December 29, 2024 23:36

github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 29, 2024

diniamo force-pushed the umu-launcher branch from 6972537 to 42fdf85 Compare December 30, 2024 01:00

diniamo changed the title ~~umu-launcher: init at 1.1.4~~ umu-launcher[-unwrapped]: init at 1.1.4 Dec 30, 2024

diniamo force-pushed the umu-launcher branch 2 times, most recently from 73f963a to 9257d01 Compare December 30, 2024 01:41

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes labels Dec 30, 2024

MattSturgeon reviewed Jan 4, 2025

View reviewed changes

diniamo force-pushed the umu-launcher branch from 9257d01 to 33d5d0a Compare January 4, 2025 07:02

This comment was marked as resolved.

Sign in to view

diniamo force-pushed the umu-launcher branch from 33d5d0a to 3fb100f Compare January 11, 2025 18:03

diniamo force-pushed the umu-launcher branch from 3fb100f to f35f6fd Compare January 15, 2025 04:23

LovingMelody reviewed Jan 17, 2025

View reviewed changes

diniamo force-pushed the umu-launcher branch from f35f6fd to 72fca19 Compare January 17, 2025 05:08

MattSturgeon reviewed Jan 17, 2025

View reviewed changes

diniamo force-pushed the umu-launcher branch 3 times, most recently from 64fef4c to be2f0c5 Compare January 18, 2025 16:36

MattSturgeon suggested changes Jan 19, 2025

View reviewed changes

pkgs/by-name/um/umu-launcher-unwrapped/package.nix Outdated Show resolved Hide resolved

pkgs/by-name/um/umu-launcher-unwrapped/package.nix Outdated Show resolved Hide resolved

diniamo force-pushed the umu-launcher branch from be2f0c5 to f552caf Compare January 19, 2025 05:09

MattSturgeon mentioned this pull request Jan 19, 2025

fix build: conditionally pass --destdir and --prefix to installer Open-Wine-Components/umu-launcher#343

Merged

MattSturgeon reviewed Jan 19, 2025

View reviewed changes

umu-launcher[-unwrapped]: init at 1.1.4

4f4441e

diniamo force-pushed the umu-launcher branch from f552caf to 4f4441e Compare January 19, 2025 13:23

MattSturgeon approved these changes Jan 19, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

MattSturgeon requested a review from khaneliman January 19, 2025 15:28

MattSturgeon added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jan 19, 2025

MattSturgeon added a commit to MattSturgeon/nix-config that referenced this pull request Jan 19, 2025

flake: switch to umu-launcher nixpkgs PR

89d24f1

Test out the nixpkgs PR instead of using the upstream flake NixOS/nixpkgs#369259

khaneliman approved these changes Jan 19, 2025

View reviewed changes

khaneliman merged commit 0d199cb into NixOS:master Jan 19, 2025
27 of 29 checks passed

This was referenced Jan 20, 2025

Refactor nix flake, based on overriding nixpkgs Open-Wine-Components/umu-launcher#345

Open

update umu-launcher: add attrs extraLibraries & extraPkgs #375423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

umu-launcher[-unwrapped]: init at 1.1.4 #369259

umu-launcher[-unwrapped]: init at 1.1.4 #369259

diniamo commented Dec 29, 2024 •

edited

Loading

diniamo commented Dec 29, 2024 •

edited

Loading

beh-10257 commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

beh-10257 commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025 •

edited

Loading

diniamo commented Jan 7, 2025

This comment was marked as resolved.

LovingMelody commented Jan 11, 2025

diniamo commented Jan 11, 2025

LovingMelody Jan 17, 2025

diniamo Jan 17, 2025

MattSturgeon Jan 17, 2025

diniamo Jan 17, 2025

LovingMelody Jan 17, 2025

diniamo Jan 18, 2025 •

edited

Loading

NullCub3 Jan 18, 2025

diniamo commented Jan 17, 2025

MattSturgeon left a comment

MattSturgeon commented Jan 17, 2025 •

edited

Loading

diniamo commented Jan 18, 2025

diniamo commented Jan 19, 2025

MattSturgeon Jan 19, 2025 •

edited

Loading

MattSturgeon Jan 19, 2025 •

edited

Loading

MattSturgeon commented Jan 19, 2025

MattSturgeon left a comment

This comment was marked as outdated.

khaneliman commented Jan 19, 2025

diniamo commented Jan 19, 2025

	extraLibraries = pkgs: let
	prevLibs = if prev ? extraLibraries then prev.extraLibraries pkgs else [ ];
	additionalLibs = with config.hardware.graphics;
	if pkgs.stdenv.hostPlatform.is64bit
	then [ package ] ++ extraPackages
	else [ package32 ] ++ extraPackages32;
	in prevLibs ++ additionalLibs;

		# Ideally, a change should be upstreamed, that adds both arguments
		# if their respective variables are set.

	# Ideally, a change should be upstreamed, that adds both arguments
	# if their respective variables are set.
	# See https://github.com/Open-Wine-Components/umu-launcher/pull/343

umu-launcher[-unwrapped]: init at 1.1.4 #369259

umu-launcher[-unwrapped]: init at 1.1.4 #369259

Conversation

diniamo commented Dec 29, 2024 • edited Loading

Things done

diniamo commented Dec 29, 2024 • edited Loading

beh-10257 commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

beh-10257 commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025 • edited Loading

diniamo commented Jan 7, 2025

This comment was marked as resolved.

LovingMelody commented Jan 11, 2025

diniamo commented Jan 11, 2025

LovingMelody Jan 17, 2025

Choose a reason for hiding this comment

diniamo Jan 17, 2025

Choose a reason for hiding this comment

MattSturgeon Jan 17, 2025

Choose a reason for hiding this comment

diniamo Jan 17, 2025

Choose a reason for hiding this comment

LovingMelody Jan 17, 2025

Choose a reason for hiding this comment

diniamo Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

NullCub3 Jan 18, 2025

Choose a reason for hiding this comment

diniamo commented Jan 17, 2025

MattSturgeon left a comment

Choose a reason for hiding this comment

MattSturgeon commented Jan 17, 2025 • edited Loading

diniamo commented Jan 18, 2025

diniamo commented Jan 19, 2025

MattSturgeon Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

MattSturgeon Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

MattSturgeon commented Jan 19, 2025

MattSturgeon left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

khaneliman commented Jan 19, 2025

nixpkgs-review result

x86_64-linux

aarch64-linux

diniamo commented Jan 19, 2025

diniamo commented Dec 29, 2024 •

edited

Loading

diniamo commented Dec 29, 2024 •

edited

Loading

MattSturgeon commented Jan 7, 2025 •

edited

Loading

diniamo Jan 18, 2025 •

edited

Loading

MattSturgeon commented Jan 17, 2025 •

edited

Loading

MattSturgeon Jan 19, 2025 •

edited

Loading

MattSturgeon Jan 19, 2025 •

edited

Loading

`nixpkgs-review` result

`x86_64-linux`

`aarch64-linux`